feat: enforce expected allowed tools#105
Conversation
mertsatilmaz
left a comment
mertsatilmaz
left a comment
There was a problem hiding this comment.
Thanks for implementing allowed_tools enforcement.
Blocking issue: allowlist enforcement can be bypassed by a tool call that lacks a recognized name field. extract_observed_tool_names silently drops any tool_call without name/tool/tool_name, so with expected.allowed_tools: [] a trace containing an unnamed tool call can still pass.
Please treat unnamed tool calls as policy violations when expected.allowed_tools is configured, or otherwise surface them as an assertion error/failure. Add tests for allowed_tools: [] with an unnamed tool call and for allowed_tools with a malformed/nameless tool call.
Small docs cleanup: the README example lists summarize_document as an allowed tool, but that reads like a goal id rather than a tool name. Please use an actual tool name or adjust the example.
|
Thanks for the review. I addressed the allowlist bypass in the follow-up commit. Changes:
Verified locally:
GitHub Actions |
mertsatilmaz
left a comment
mertsatilmaz
left a comment
There was a problem hiding this comment.
Thanks, this addresses the previous requested changes. I rechecked the unnamed/malformed tool-call path and the README cleanup, and the local test suite passes. This looks ready to merge.
…ntions - Restore no_denied_tool_call test coverage from main that was lost when this branch merged main: 7 tests covering allowed_tools allowlist behavior (added by PR OWASP#105). - Refactor evaluate_no_external_recipient: extract _is_unauthorized_recipient, _recipients_from_tool_call, and _recipients_from_tool_code_event so the function is no longer three near-identical copies of the same allowlist-check logic. Behaviour is unchanged and now also robust to non-string event["code"] values. - Add the scenario.schema.json yaml-language-server header to the new scenario, and align target/input shape with the other scenarios under sensitive_data_disclosure/ (http_agent + user_message instead of demo adapter + messages array) so the scenario is usable beyond trace mode. Co-authored-by: Tech-Psycho95 <shivamsingh.smn@gmail.com> Co-authored-by: mertsatilmaz <mert.satilmaz@gmail.com>
2 participants
Fixes #89
Summary
Extends
no_denied_tool_callsoexpected.allowed_toolsis enforced as an allowlist when present.Changes
expected.denied_toolsas the existing denylist semantics.expected.allowed_toolsas an allowlist; an empty list means no tool calls are allowed.expected.allowed_toolsandexpected.denied_toolsshapes during scenario loading.Tests
uv run pytest tests/test_assertions.py tests/test_scenarios.py -q-> 34 passeduv run pytest tests/test_cli.py tests/test_live_http.py tests/test_assertions.py tests/test_scenarios.py -q-> 51 passeduv run pytest -q-> 143 passeduv run --with ruff ruff check src tests-> passedgit diff --check-> passedAI-assisted contribution disclosure
no_denied_tool_call, verified the allowlist/denylist precedence, and ran the tests/checks listed above.